Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate new platform (core) server side into Kibana #18951

Merged
merged 182 commits into from
Jul 11, 2018
Merged

Integrate new platform (core) server side into Kibana #18951

merged 182 commits into from
Jul 11, 2018

Conversation

azasypkin
Copy link
Member

@azasypkin azasypkin commented May 9, 2018

In this PR we're integrating base parts of the new platform (later core) server side into Kibana. At this point there are only two major touch points between new core and the rest of Kibana:

  • Now it's the core who listens on HTTP ports, handles TLS configuration and base path proxy. All requests to Kibana will hit HTTP server exposed by the core first and it will decide whether request can be solely handled by the new platform or request should be forwarded to the legacy Kibana. Technically we have two Hapi servers now (one for core and one for legacy Kibana), but only one underlying Node HTTP listener. This setup allows us to gradually introduce any "pre-route" processing code, expose new routes or replace old ones handled by the legacy platform currently.
  • Once config has been validated by the legacy Kibana, part of it will also be validated by the new core so that we can make config validation stricter with the new config validation system (even though it's based on Joi internally we introduced custom rules tailored to our needs like byteSize or duration). That means you can notice new config validation error messages if config value has been accepted by legacy config validation code, but rejected by the core.

With this PR legacy Kibana will still be started first using existing CLI and will bootstrap core only when needed, but we're going to inverse this in #19994.


New platform formerly resided in new-platform feature branch and then major part of it has been migrated to kbn-core feature branch. new-platform feature branch is going to stay for a while so that we can migrate the missing parts when we're ready. Here is a list of notable kbn-core vs new-platform differences:

kimjoar and others added 30 commits June 20, 2017 23:26
#12687)

* Rename 'xpack_api_polling_frequency_millis' to 'api_polling_frequency'

* prettier
* Cleaner Pid observable code

* remove unused arg
* Implement mapOf setting type

* better variable names

* sharing code
@azasypkin azasypkin requested a review from spalger June 29, 2018 22:01
Copy link
Contributor

@epixa epixa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was reviewed over the last year in a long series of individual PRs. I just did a high level once over on the code to double check, and it looks good.

Please update the PR title and description though. To most people, this PR represents the first addition of the new platform. Can you update this PR to detail the changes?

LGTM

@rhoboat
Copy link

rhoboat commented Jul 10, 2018

Question about the kbn-observable that we changed into src/core/lib/kbn_observable: why didn't we move the share_last operator over? Should we make a note about this, in case we plan to reimplement that? I assume we don't want it anymore at all?

Copy link

@rhoboat rhoboat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we either add some follow up about the stuff that we didn't pull over from kbn-observable, or include them here? If this gets merged as is, the things that didn't make it will be lost/forgotten.

@epixa
Copy link
Contributor

epixa commented Jul 10, 2018

@archanid Are you referring to kbn-observable functionality from the new-platform branch that wasn't included here? If so, that shouldn't block this change.

The new-platform branch was abandoned the moment we created kbn-core, it continues to exist only so we can reference and potentially scavenge pieces from it in the future if we need to. So if there is a feature from it that you'd like to use going forward, you can copy it from that branch and PR to master just like any other change.

@rhoboat
Copy link

rhoboat commented Jul 10, 2018

@epixa yes, indeed.
Hm, but why didn't we include everything but that one operator? I just wonder. The description says we pulled the whole kbn-observable lib in here. Is it missing, or deliberately omitted?

@epixa
Copy link
Contributor

epixa commented Jul 10, 2018

@archanid Our stance with code has always been to delete it if it's not being used, and it's not being used. I'm not sure if that's why it was omitted, but it's why I would have omitted it had I made the change :)

@azasypkin
Copy link
Member Author

azasypkin commented Jul 10, 2018

Question about the kbn-observable that we changed into src/core/lib/kbn_observable: why didn't we move the share_last operator over? Should we make a note about this, in case we plan to reimplement that? I assume we don't want it anymore at all?

I just removed everything we don't need in the initial merge, and it was the only operator that depended on RxJS, but not used elsewhere. Since we have this in new-platform (as well as other pieces like elasticsearch client, plugins support etc.) we can pull it in when/if we need it.

@epixa
Copy link
Contributor

epixa commented Jul 10, 2018

@archanid woo!

@azasypkin Please update the title/description and fix the dependency conflicts and get this beautiful beast merged!

@azasypkin azasypkin changed the title Merge kbn-core into master Integrate new platform (core) server side into Kibana Jul 11, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@azasypkin azasypkin merged commit 1418310 into master Jul 11, 2018
azasypkin added a commit to azasypkin/kibana that referenced this pull request Jul 11, 2018
Co-authored-by: Kim Joar Bekkelund <kjbekkelund@gmail.com>
Co-authored-by: archana <archanid@users.noreply.github.com>
Co-authored-by: Spencer <spalger@users.noreply.github.com>
Co-authored-by: Court Ewing <court@epixa.com>
@azasypkin
Copy link
Member Author

6.x/6.4: f88d0b9

@elasticmachine
Copy link
Contributor

💔 Build Failed

@azasypkin
Copy link
Member Author

azasypkin commented Jul 11, 2018

Hmm, why CI is still running for this PR? :) Anyway it seems CI failed because of flaky test... Will be monitoring master today.

@w33ble
Copy link
Contributor

w33ble commented Jul 13, 2018

This PR broke Canvas. We just see a bunch of 404 errors in the console and nothing loads. There's an extra slash in the basepath now, not sure if that's related.

screenshot 2018-07-13 15 00 00


I opened #20802

@azasypkin azasypkin deleted the kbn-core branch July 24, 2018 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v6.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants